Skip to content

Validate the configuration map before persisting it (fixes #520) - #558

Open
iacsha wants to merge 1 commit into
OpenIntegrationEngine:mainfrom
iacsha:fix/520-configmap-validate-before-persist
Open

Validate the configuration map before persisting it (fixes #520)#558
iacsha wants to merge 1 commit into
OpenIntegrationEngine:mainfrom
iacsha:fix/520-configmap-validate-before-persist

Conversation

@iacsha

@iacsha iacsha commented Nov 4, 2026

Copy link
Copy Markdown

Validate the configuration map before persisting it (fixes #520)

Problem

PUT /api/server/configurationMap does not check the map it receives. The XML/JSON
body readers deserialize into a raw map, so the body can produce a null map, a null
key, a non-string key, or a null / non-ConfigurationProperty value.

DefaultConfigurationController.setConfigurationProperties persists the map before
it builds the in-memory copy:

if (persist) {
    saveConfigurationProperties(map);   // saved first
}
...
for (Entry<String, ConfigurationProperty> entry : map.entrySet()) {
    valueMap.put(entry.getKey(), entry.getValue().getValue());  // then throws NPE/CCE here

So a bad map is written to the CONFIGURATION table first, and the in-memory update
then throws. With configurationmap.location = database this leaves a row that breaks
the configuration map on the next startup — the request 500s but the damage is already
persisted.

Fix

  • Add validateConfigurationMap(map): rejects a null map, and any entry with a
    non-string key or a null / non-ConfigurationProperty value, throwing
    ControllerException before anything else.
  • Reorder setConfigurationProperties to validate → build the in-memory maps →
    persist last    , so an invalid map can never reach the database.

The database-load path (loadDatabaseConfigPropsIfNecessary) already catches
ControllerException and logs it, so a server already bricked by this bug now starts
and skips the bad map instead of failing to load.

Tests

Added to DefaultConfigurationControllerTest:

  • null map throws ControllerException
  • entry with a null value throws
  • non-string key throws
  • a valid map is applied

./gradlew :server:test --tests "*DefaultConfigurationControllerTest*" → 19 tests, 0
failures.

Not included

The deeper fix — making the XML/JSON body readers honor the map's generic types so a
raw map can't be constructed in the first place — is left for a follow-up to keep this
change focused on preventing the persistence corruption.

mgaffigan
mgaffigan requested changes Nov 4, 2026
View reviewed changes

@mgaffigan mgaffigan left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to use the same validation implementation on get as we do put, lest we get desynchronized.

Also: DCO

@iacsha
Validate the configuration map before persisting it (fixes OpenIntegr…
1747960 
…ationEngine#520)

setConfigurationProperties saved the map before building the in-memory copy, so an invalid PUT body was written to the database and then threw, corrupting the configuration map on restart. Validate the map first, build the in-memory maps, and persist last.

Signed-off-by: iacsha <122627209+iacsha@users.noreply.github.com>
@iacsha
iacsha force-pushed the fix/520-configmap-validate-before-persist branch from 19185b5 to 1747960 Compare November 6, 2026 19:47
@iacsha

iacsha commented Nov 6, 2026

Copy link
Copy Markdown
Author

Thanks @mgaffigan. DCO sign-off is fixed on all my open PRs.

On the get/put point: I think they already share one implementation. validateConfigurationMap is called at the top of setConfigurationProperties, and that method is the single path for all three ways a map comes in:

  • the API PUT (ConfigurationServlet.setConfigurationMap)
  • the database load in loadDatabaseConfigPropsIfNecessary (it deserializes the stored map and calls setConfigurationProperties(map, true))
  • the file load in initialize() (setConfigurationProperties(configurationMap, false))

So a map read back from the database is checked by the same code that checks a PUT, and they can't drift apart.

One gap I did notice on the read side: if a bad map was already stored before this fix, the load path catches the ControllerException, logs it, and carries on with an empty map. A ClassCastException from the HashMap cast isn't caught at all. If that is closer to what you meant, I'm happy to add it here so a bad stored map fails clearly instead of falling back to empty. Let me know which way you'd like it.

@github-actions

github-actions Bot commented Nov 7, 2026

Copy link
Copy Markdown

Test Results

  141 files  ±0    141 suites  ±0   3m 43s ⏱️ +44s
  923 tests +4    923 ✅ +4  0 💤 ±0  0 ❌ ±0 
1 055 runs  +4  1 049 ✅ +4  6 💤 ±0  0 ❌ ±0 

Results for commit 1747960. ± Comparison against base commit 892e33f.

@tonygermano

tonygermano commented Nov 7, 2026

Copy link
Copy Markdown
Member

This is a duplicate of #552 . I don't mind if you want to take over the fix, but please compare this to the existing PR to see if any part of it should be included in yours. It's still in a draft because it is stacked on top of #515 and was waiting for that one to merge to rebase. I don't remember exactly what's in it now, but I think it adds some tests that depend on 515 merging first.

Additionally, new contributors are being asked to sign the Eclipse Contributor Agreement. It is now described in section 8 of CONTRIBUTING.md. The failing check on all of your PRs should go away once you have completed that step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mgaffigan mgaffigan mgaffigan approved these changes
@kayyagari kayyagari Awaiting requested review from kayyagari kayyagari is a code owner automatically assigned from OpenIntegrationEngine/maintainers
@gibson9583 gibson9583 Awaiting requested review from gibson9583 gibson9583 is a code owner automatically assigned from OpenIntegrationEngine/maintainers
@ssrowe ssrowe Awaiting requested review from ssrowe ssrowe is a code owner automatically assigned from OpenIntegrationEngine/maintainers
@kpalang kpalang Awaiting requested review from kpalang kpalang is a code owner automatically assigned from OpenIntegrationEngine/maintainers
@jonbartels jonbartels Awaiting requested review from jonbartels jonbartels is a code owner automatically assigned from OpenIntegrationEngine/maintainers
@NicoPiel NicoPiel Awaiting requested review from NicoPiel NicoPiel is a code owner automatically assigned from OpenIntegrationEngine/maintainers
@tonygermano tonygermano Awaiting requested review from tonygermano tonygermano is a code owner automatically assigned from OpenIntegrationEngine/maintainers

At least 3 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] Invalid configuration map PUT body is saved to the database, and the map is empty after a restart

3 participants

@iacsha @tonygermano @mgaffigan